thermal-service: Separate interface from implementation#777
thermal-service: Separate interface from implementation#777kurtjd wants to merge 1 commit intoOpenDevicePartnership:feature/relay-splitfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures the thermal subsystem by splitting the public interface into a dedicated crate, moving MCTP relay/serialization into a separate relay crate, and refactoring the thermal-service implementation to use direct async calls instead of IPC.
Changes:
- Introduces
thermal-service-interfacetraits for sensor/fan/thermal services and updates thermal-service to implement them. - Adds
thermal-service-relayimplementing MCTP request/response types + a relay handler wrapper around aThermalService. - Refactors thermal-service sensor/fan services to remove IPC channels and use async method calls + event senders.
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
uart-service/Cargo.toml |
Switches dependency from legacy thermal messages crate to the new interface crate. |
thermal-service/src/utils.rs |
Minor doc/comment cleanup; removes Celsius/Kelvin conversion helpers from this crate. |
thermal-service/src/sensor.rs |
Replaces IPC-based sensor device/service with an async trait-based sensor service + event broadcasting. |
thermal-service/src/fan.rs |
Replaces IPC-based fan device/service with an async trait-based fan service + event broadcasting. |
thermal-service/src/lib.rs |
Turns thermal-service into a registry/handle over sensor+fan service slices implementing interface traits. |
thermal-service/src/mptf.rs |
Removes in-crate MPTF message handling (moved to relay crate). |
thermal-service/src/context.rs |
Removes IPC/event-queue-based context layer (no longer needed). |
thermal-service/src/mock/* |
Updates mocks to implement new interface driver traits; adds helper configs; removes old convenience wrappers. |
thermal-service/Cargo.toml |
Replaces thermal-service-messages with thermal-service-interface + thermal-service-relay. |
thermal-service-interface/* |
New crate defining ThermalService, SensorService, FanService, driver traits, and event/error types. |
thermal-service-relay/src/* |
New crate providing MPTF serialization + ThermalServiceRelayHandler<T> wrapper. |
thermal-service-relay/Cargo.toml |
Defines relay crate deps/features. |
embedded-service/src/relay/mod.rs |
Relaxes RelayServiceHandler::process_request future bound (drops Send). |
embedded-service/src/event.rs |
Adds Sender/Receiver trait impls for embassy_sync::channel sender/receiver types. |
examples/std/src/bin/thermal.rs |
Updates example to spawn separate sensor/fan services, consume events, and access via thermal service handle. |
examples/std/Cargo.toml / Cargo.lock |
Updates example deps to new interface/relay crates. |
Cargo.toml / Cargo.lock |
Adds new workspace members and dependency mappings for interface/relay crates. |
Comments suppressed due to low confidence (1)
thermal-service-relay/Cargo.toml:14
- This crate declares a dependency on
time-alarm-service-messages, but there are no references to it inthermal-service-relaysources. With workspacewarnings = deny, an unused dependency can break builds depending on lint settings, and it increases the dependency surface unnecessarily.
Remove this dependency if it’s not needed, or add the missing usage if it is required.
kurtjd
force-pushed
the
thermal-interface
branch
from
f4c5a92 to
36a10a6
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
36a10a6 to
c88f3c3
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
c88f3c3 to
a60d7e6
Compare
kurtjd
requested review from
RobertZ2011,
Copilot,
felipebalbi,
tullom and
williampMSFT
kurtjd
force-pushed
the
thermal-interface
branch
from
a60d7e6 to
bf19365
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
2fca3ff to
e514842
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
e514842 to
667132a
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
667132a to
ecb5214
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
thermal-service/src/sensor.rs:34
with_retry!never returnssensor::Error::Hardware; any underlying bus error or timeout is retried until attempts are exhausted and then surfaced asRetryExhausted. This makes it impossible for callers/events to distinguish a persistent hardware failure from a retry exhaustion condition, and leaves theHardwarevariant effectively unused. Consider tracking the last failure cause and returningHardwarefor immediate/permanent bus errors (or remove theHardwarevariant if it’s not intended to be reported).
let mut retry_attempts = $self.config.lock().await.retry_attempts;
loop {
if retry_attempts == 0 {
break Err(sensor::Error::RetryExhausted);
}
match with_timeout(BUS_TIMEOUT, $bus_method).await {
Ok(Ok(val)) => break Ok(val),
_ => {
retry_attempts -= 1;
}
ecb5214 to
eb04c3b
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
eb04c3b to
02e154a
Compare
kurtjd
force-pushed
the
thermal-interface
branch
from
02e154a to
c3d2d9d
Compare
|
@jerrysxie @williampMSFT @felipebalbi I think I've addressed everyone's feedback as best I could. Please resolve conversations if my reasoning/fixes are adequate. For suggestions out of scope of this PR, I've created several issues to track them: |
@kurtjd I would recommend adding commits instead of amending commits since we utilize squash now. It is a bit hard for reviewers to figure out what new changes are layered on already reviewed changes. |
Sounds good, I will do that from now on. |
kurtjd
force-pushed
the
thermal-interface
branch
from
c3d2d9d to
f6fd04a
Compare
This PR overhauls the thermal-service a bit. Essentially, the thermal interface has been split from the implementation into a trait, IPC has been removed and replaced with direct async calls, and a relay service handler wrapper has been introduced.
Instead of one monolithic "thermal service", there are really 3 services here: sensor service, fan service, and thermal service, each with their own interfaces. The thermal service now mainly exists as the relay handler, since it can keep track of the different spawned sensor and fan services by ID, therefore knowing which sensors and fans to talk to when a request comes from the host.
Also split into a thermal service relay handler type which is intended to wrap any thermal service implementation.
There are some things I will likely want to tweak in the future based off some work Robert has been doing such as better use of some of the event sender/receiver traits and use of the Lockable trait (I heavily use interior mutability with direct dep on embassy Mutex, not sure if I can somehow make this generic over the Lockable trait?).
With the removal of IPC, need to also think about how to handle heterogeneous collections of sensors and fans. Currently this design just assumes the user will need to handle that on their own, by introducing enum dispatch or something behind the scenes, but will investigate if there are ways to make that easier (such as using the
enum_dispatchcrate and marking these traits).This is of course a large breaking change so will announce on Zulip after merge.
Resolves #756
Resolves #781